330397 - Additional problems with incomplete cache entries and cache flush assertions.

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Darin Fisher]

Additional problems with incomplete cache entries and cache flush assertions.

This bug is basically about follow-up for my patch for bug 189570.

Comment 1

[Darin Fisher]

Attachment: v1 patch

This patch cleans up a bunch of stuff.  The logic for dooming a cache entry now widdles down to whether or not we have initialized it.  If we receive READ+WRITE access to the cache entry, then we can assume that it was previously initialized.  If we only have WRITE access, then we were responsible for initializing it.  The mOpenedCacheForWriting flag is then replaced with a mInitedCacheEntry flag, that is set if InitCacheEntry succeeds.

Comment 2

[Darin Fisher]

Oh, the change to nsDiskCacheStreams.cpp is to avoid an assertion resulting from trying to flush a doomed entry.  Doomed entries are no longer present in the _CACHE_MAP_, so it makes no sense to try to flush their data to disk.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 214950 [details] [diff] [review]
v1 patch

sr=bzbarsky if us no longer checking the IsResumable() thing is ok.

Comment 4

[Darin Fisher]

Boris, we check for that inside CheckCache.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  ;)

Comment 6

[Darin Fisher]

fixed-on-trunk

Comment 7

[Bill Gianopoulos [:WG9s]]

With this fix included, I am unable to download executable files using firefox.  The file appears to be truncated as if the final buffer was not flushed.  Backing out this patch corrects the issue.  There are many complaints about this issue using the Firefox trunk 3/22 nightly on the Mozillazine forums.

Comment 8

[Darin Fisher]

Hmm... ah, I suspect there may be some 'cache as file' consumers who expect to be able to doom the cache entry and still access the cache file while holding onto the cache entry descriptor.  Yeah, that could explain the regression.  In that case, I think I'll need to simply suppress editing the cache map when the entry is doomed.  OK, backing out the v1 patch for now.

Comment 9

[pal-moz]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060322 Firefox/1.6a1 ID:2006032301

first time to download an exe file is failed.
but second time to download the same file is complete.

this seems to be related to cache, but why the first time is failed.

now this checkin is backed out, so 0323 build is almost same with 0321 build (before this checkin landed.), why can not download at first time ?

Comment 10

[Bill Gianopoulos [:WG9s]]

Hmm, it appears I was wrong in comment #7.  I stupidly forgot to clear the cache before redoing my test downloads after backing out the patch from my tree. So, it would appear that it is a different checkin that caused the download regression.  Unfortunately, none of them really jump out as looking likely.

Comment 11

[Bill Gianopoulos [:WG9s]]

Perhaps it is really caused by bug 162361?  Unfortunately I don't have time to do a build right now.

Comment 12

[Bill Gianopoulos [:WG9s]]

OK. I decided I did have time to do a build.  I reapplied this patch and backed out 162361.  Then I removed the truncated files and cleared the cache and was able to successfully download my test files on the fist try.

Sorry for my previous misinformation. :-(

Comment 13

[Bill Gianopoulos [:WG9s]]

Forgot to mention, I filed bug 331453 on the download issue, and made it block 162361.  Sorry for the bugspam.

Comment 14

[Darin Fisher]

Attachment: v1.1 patch -- version checked in

I modified the cache changes slightly to account for the possibility of a doomed cache entry that is being 'streamed as file'

Comment 15

[Darin Fisher]

fixed-on-trunk